Skip to content

feat: hmr, compare old locals to invalidate if needed #692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

feat: hmr, compare old locals to invalidate if needed #692

wants to merge 3 commits into from
+67 −13

Conversation

goloveychuk
Copy link

@goloveychuk goloveychuk commented Feb 2, 2021 •
edited
Loading

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

This allows self accept css module if locals didn't changed. If changed - propagate.
Inspired by https://github.com/webpack-contrib/style-loader/blob/master/src/index.js#L231
Now new runtime is

// extracted by mini-css-extract-plugin
    if(true) {
      // 1612290403401
      var cssReload = __webpack_require__(/*! ../node_modules/mini-css-extract-plugin/dist/hmr/hotModuleReplacement.js */ "../node_modules/mini-css-extract-plugin/dist/hmr/hotModuleReplacement.js")(module.id, {"publicPath":"","esModule":false,"locals":false});
      var _locals = undefined;
      var isEqualLocals = __webpack_require__(/*! ../node_modules/mini-css-extract-plugin/dist/hmr/isEqualLocals.js */ "../node_modules/mini-css-extract-plugin/dist/hmr/isEqualLocals.js");
      module.hot.dispose(function(data) {
        cssReload();
        data.oldLocals = _locals;
      });
      
      if (!_locals || module.hot.invalidate) {
        if (module.hot.invalidate &&
            module.hot.data &&
            module.hot.data.oldLocals &&
            !isEqualLocals(module.hot.data.oldLocals, _locals)) {
            module.hot.invalidate();
        } else {
          module.hot.accept();
        }
      }
    }
  //# sourceURL=[module]
//# sourceMappingURL=data:applicati

Breaking Changes

Additional Info

@goloveychuk
Copy link
Author

@alexander-akait could you please review? Should I fix tests first (look like all of them are code snapshots)

alexander-akait
alexander-akait reviewed Feb 5, 2021
View reviewed changes
src/loader.js Outdated
@@ -30,7 +38,15 @@ function hotLoader(content, context) {
...context.options,
locals: !!context.locals,
})});
module.hot.dispose(cssReload);
var _locals = ${JSON.stringify(context.locals)};
var isEqualLocals = require(${loaderUtils.stringifyRequest(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When esModule is true it should be import,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but previously it has

var cssReload = require(${loaderUtils.stringifyRequest(
        context.context,
        path.join(__dirname, 'hmr/hotModuleReplacement.js')

This should be import too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

goloveychuk
goloveychuk commented Feb 5, 2021
View reviewed changes
src/loader.js
const request = loaderUtils.stringifyRequest(context.context, modulePath);

if (context.esModule) {
return `import ${name} from ${request};`;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-akait not sure if webpack module interop will work here, mb need to change to * as $name

@goloveychuk
Copy link
Author

printed output,
image

@goloveychuk
Copy link
Author

@alexander-akait could you please review?

@alexander-akait
Copy link
Member

@goloveychuk Sorry for delay, apparently I missed a notification somewhere 😞 , do not hesitate to ping me again, do you want to finish this?

@alexander-akait alexander-akait mentioned this pull request Apr 16, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-akait alexander-akait Awaiting requested review from alexander-akait

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@goloveychuk @alexander-akait